I haven't added histogram specific arguments like bins, binwidth etc, and comparator arguments for testing like greater, greater_or_equal etc. Do we need to add these as well? @edublancas

@neelasha23 I think expanding the parameters when we read the arguments string would be better so we automatically add expansion to them. Otherwise, we'll have to remember to add this feature to every argument we add. If you look at the SQL magic implementation, there should be a part where we obtain the raw argument string e.g. --a something --b another; we could expand the values there

Addressed the comment. Please review @edublancas

@bryannho please review this, pay special attention to this comment

@edublancas @neelasha23 Do we want to allow users to specify argument flags using variables in this way? I assume this wasn't the original intent. It could definitely be helpful but might also open up more issues in the future.

I think for this PR we should only handle arguments and not flags. I have changed the approach. Previously I had tried a couple of approaches:

1. Regex based to extract --arg {{value}} patterns and then rendering. This works but I'm not very confident of this approach since false edge cases/ missed cases might come up.
2. Allow the argparse parser to parse the input as is and then substitute the args. So the argparse.Namespace would contain values like save: {{snippet}} and then {{snippet}} would be rendered by the actual snippet name. I feel this approach is less error prone and more deterministic, as now we are rendering only the argument values specifically, although it requires an additional step of iterating over the namespace. So I have reverted to this approach.

Here are the changes:

columns
explore
profile
snippets
tables
test
command
magic
magic_plot
util functions
util functions

Regarding the error, now it will display error if we try to use the snippet:

@edublancas @bryannho

Allow the argparse parser to parse the input as is and then substitute the args. So the argparse.Namespace would contain values like save: {{snippet}} and then {{snippet}} would be rendered by the actual snippet name. I feel this approach is less error prone and more deterministic, as now we are rendering only the argument values specifically, although it requires an additional step of iterating over the namespace. So I have reverted to this approach.

I'm confused. this approach sounds like a much more robust alternative than adding code to each of the SQL commands, and you mentioned "I have reverted to this approach",

but then I looked at the code and there are a few snippets that look like this:

   if is_rendering_required(" ".join(others)):
        expand_args(args, user_ns)

the objective is to have a single place for expanding the {{variables}} and I thought we had agreed to do that. but it seems like we still have an implementation that requires us adding the same snippet on each command?

the objective is to have a single place for expanding the {{variables}} and I thought we had agreed to do that. but it seems like we still have an implementation that requires us adding the same snippet on each command?

What i understood from your previous comment, you mentioned that we we might forget to add every-time we add a new argument so we need a approach where we dont need to bother abut adding the arguments to a list. But in the current approach we dont need to maintain any list of that sort it automatically detects the "{{value}}" pattern inside argparse and replaces them.
But we still need to call this util function at the different commands file right? like sql, sqlplot. and the different versions of sqlcmd. Also, sqlcmd is parsed differently than than the other two.

Now if we add any new command like sqlcmd something or sqlsomething only then we need to add this block of code otherwise not. Does this make sense?

the objective is to have a single place for expanding the {{variables}} : I'm also not sure of this part either because even if we expand the string we would need to do it separately for sqlplot, sqlcmd and sql right ? because the line text will be read in that particular command file only. It would be there at 3 files. (And with this appraoch we have the problem where user might try to declare variables for any token and not just parameters) .But with current approach we need to include it in all the sqlcmd files because there are separate parser files like snippet, columns, etc. (only for sqlcmd)

If we work directly on the string we need to add multiple rules/ regex patterns to ensure we pick up only the arguments and I'm not very sure of this approach.

Let me know what you think. If you have any suggestions it would be helpful

@edublancas

@neelasha23 oh, you're right. I thought we could add a single piece of logic that would work for all commands (%%sql, %sqlcmd , etc). But then I realized that they are all implemented in different places as they are magic objects, so your approach makes sense.

just fix the conflicts, the integration tests and this should be good to go!

conflicts and tests are fixed now @edublancas